feat: support reverse and bidirectional arrows in DSL#30
feat: support reverse and bidirectional arrows in DSL#30swiftlysingh wants to merge 1 commit intomainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 27 minutes and 43 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds support in the flowchart DSL for reverse (<-) and bidirectional (<->) connections, aiming to keep graph semantics consistent while emitting the correct Excalidraw arrowhead configuration through the generator/CLI.
Changes:
- Extend DSL tokenization/parsing to recognize
<-and<->and attach arrowhead style metadata. - Adjust Excalidraw arrow creation to preserve explicit
nullarrowhead values (instead of defaulting via??). - Add unit/integration tests and document the new DSL syntax in the README.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/parser/dsl-parser.test.ts | Adds coverage for reverse/bidirectional parsing and arrowhead output (currently has ordering brittleness and expectation mismatch). |
| tests/integration/exporter/cli-export.test.ts | Validates CLI create --inline preserves arrowhead configuration (currently order-dependent). |
| src/parser/dsl-parser.ts | Implements <- / <-> tokenization and edge creation with pending style flags (contains a reverse-arrow semantic/render mismatch). |
| src/factory/connection-factory.ts | Fixes arrowhead defaulting so explicit null is preserved (enables start/end arrowhead control). |
| README.md | Documents new arrow syntaxes and provides examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ...(pendingBidirectional | ||
| ? { startArrowhead: 'arrow' as const, endArrowhead: 'arrow' as const } | ||
| : pendingReversed | ||
| ? { startArrowhead: 'arrow' as const, endArrowhead: null } | ||
| : {}), |
There was a problem hiding this comment.
For <- you’re currently (a) swapping source/target (so the edge is already B→A) and (b) also forcing startArrowhead: 'arrow' + endArrowhead: null. Those two together invert the rendered arrow direction relative to the logical edge. If the edge direction is reversed via source/target swap, the arrowheads should typically remain the default (start null, end 'arrow') or explicitly set startArrowhead: null, endArrowhead: 'arrow'—but not start-only.
| expect(result.edges[0].target).toBe(result.nodes[0].id); | ||
| expect(result.edges[0].style).toEqual({ | ||
| startArrowhead: 'arrow', | ||
| endArrowhead: null, | ||
| }); |
There was a problem hiding this comment.
This test asserts startArrowhead: 'arrow', endArrowhead: null for a reverse connection while also asserting that source/target are swapped (logical B→A). That combination makes the arrowhead placement inconsistent with the edge direction in Excalidraw (single-headed arrows normally use endArrowhead: 'arrow' on the target side). Consider updating the expected style to default arrowheads (start null, end 'arrow') or otherwise align with whatever convention you choose (either swap endpoints or flip arrowheads, but not both).
| const arrows = file.elements.filter((element) => element.type === 'arrow'); | ||
|
|
||
| expect(arrows).toHaveLength(2); | ||
| expect(arrows[0]).toMatchObject({ startArrowhead: 'arrow', endArrowhead: null }); | ||
| expect(arrows[1]).toMatchObject({ startArrowhead: 'arrow', endArrowhead: 'arrow' }); |
There was a problem hiding this comment.
This test relies on arrows[0] / arrows[1] ordering after running ELK layout. layoutGraph() builds layoutedEdges by iterating layoutResult.edges, and ELK doesn’t guarantee stable edge ordering, so this can be flaky. Prefer asserting on the multiset of {startArrowhead,endArrowhead} pairs (order-independent) or locating each arrow by its bound element IDs / edge IDs.
| const arrows = file.elements.filter((element: { type: string }) => element.type === 'arrow'); | ||
|
|
||
| expect(arrows).toHaveLength(2); | ||
| expect(arrows[0]).toMatchObject({ startArrowhead: 'arrow', endArrowhead: null }); | ||
| expect(arrows[1]).toMatchObject({ startArrowhead: 'arrow', endArrowhead: 'arrow' }); |
There was a problem hiding this comment.
This test assumes the first/second arrow element corresponds to a specific edge. Since edge ordering can change during ELK layout (and therefore element emission order), asserting on arrows[0] vs arrows[1] may be flaky. Make the assertion order-independent (e.g., compare sorted arrowhead pairs or find arrows by edge id / bindings). Also ensure the expected arrowhead placement for <- aligns with the chosen semantics (swap endpoints vs flip arrowheads).
Summary
<-and<->Testing
[C] <-> [D]" -o tests/tmp/manual/arrow-directions.excalidraw